feat: enforce IPSIE session_expiry ceiling on local session lifetime#1126
Conversation
yogeshchoudhary147
force-pushed
the
feat/ipsie-session-expiry
branch
3 times, most recently
from
63added to
6814d11
Compare
| - `user` becomes `undefined` | ||
| - `getAccessTokenSilently()` returns `undefined` (no error thrown) | ||
|
|
||
| If your routes are wrapped with `withAuthenticationRequired`, no code changes are required — the state change triggers a redirect to login automatically. Components that call `getAccessTokenSilently()` imperatively (e.g. in a click handler or `useEffect`) need an explicit null check; see [Upgrading existing apps](#upgrading-existing-apps) below. |
There was a problem hiding this comment.
The ceiling is only checked when something calls getAccessTokenSilently, getUser, or getIdTokenClaims again , there's no timer or background re-check in the provider, and isAuthenticated only changes when one of those runs and re-reads the user. So a user sitting on an already-rendered protected page past the ceiling stays authenticated until the next such call; the withAuthenticationRequired redirect fires off the isAuthenticated change, which won't happen on a passive page.
Could we soften "reflects the expired state on the next render" and "automatically" here so it reads as "on the next token/user call" rather than real-time? Otherwise it sets an expectation the SDK doesn't hold.
There was a problem hiding this comment.
Good catch — updated. Changed "on the next render" to "on the next call to getAccessTokenSilently, getUser, or getIdTokenClaims" and added a note that a passive page won't trigger the state change. Dropped "automatically" from the withAuthenticationRequired description too.
| }); | ||
|
|
||
| describe('session_expiry ceiling (IPSIE SL1)', () => { | ||
| it('should return undefined and clear auth state when session ceiling is breached during getAccessTokenSilently', async () => { |
There was a problem hiding this comment.
I doesn't mock a token call actually happened ,a regression that short-circuits before calling getTokenSilently would still pass. It also doesn't exercise the ceiling itself, since enforcement lives in the underlying SDK.
Can we add an expect(clientMock.getTokenSilently).toHaveBeenCalled() and rename the test to say that, rather than "session_expiry ceiling" which implies it tests detection?
There was a problem hiding this comment.
Fair — renamed the describe/it away from "ceiling" since we're not testing detection (that's spa-js's job), and added expect(clientMock.getTokenSilently).toHaveBeenCalled().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR bumps ChangesSession Expiry Support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
EXAMPLES.md (1)
1623-1623: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor redundancy flagged by static analysis.
"this point in time" — consider trimming to "this point" or "this time".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@EXAMPLES.md` at line 1623, The sentence in EXAMPLES.md contains a small redundancy in the `session_expiry` description. Update the text in the relevant enterprise connection explanation to trim “this point in time” to a shorter phrasing like “this point” or “this time,” keeping the meaning unchanged and preserving the surrounding `id_token_session_expiry_supported` and `session_expiry` context.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@EXAMPLES.md`:
- Line 1623: The sentence in EXAMPLES.md contains a small redundancy in the
`session_expiry` description. Update the text in the relevant enterprise
connection explanation to trim “this point in time” to a shorter phrasing like
“this point” or “this time,” keeping the meaning unchanged and preserving the
surrounding `id_token_session_expiry_supported` and `session_expiry` context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c526e11-06d3-4592-b4eb-cd7390fcdef7
📒 Files selected for processing (3)
EXAMPLES.md__tests__/auth-provider.test.tsxpackage.json
yogeshchoudhary147
force-pushed
the
feat/ipsie-session-expiry
branch
from
06c5a16 to
be018e1
Compare
2 participants
Summary
@auth0/auth0-spa-jsto^2.22.0to pick up IPSIEsession_expiryenforcementgetAccessTokenSilentlySession Expiry from Upstream IdP (IPSIE)section toEXAMPLES.mdNo provider code changes are needed. Enforcement is fully handled by
auth0-spa-js. The React layer already propagates theundefineduser/token responses intoisAuthenticated: falsestate transitions. Routes wrapped withwithAuthenticationRequiredrequire no code changes; components callinggetAccessTokenSilently()imperatively need an explicit null check (documented in EXAMPLES.md).Test plan
session_expiry ceiling (IPSIE SL1)test passesapi.idToken.setCustomClaim('session_expiry', Math.floor(Date.now() / 1000) + 120), log in, wait 2 minutes. The app should redirect to login without any code changes.Summary by CodeRabbit
Documentation
Bug Fixes
getAccessTokenSilently()returnsundefined.Tests